-
Notifications
You must be signed in to change notification settings - Fork 267
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(nextcloud): add notify_push support #581
base: main
Are you sure you want to change the base?
Conversation
4c3cee5
to
46eb0e9
Compare
f5f716e
to
6ed673a
Compare
Can we ensure that the notify_push-plugin is installed? Maybe something like this? lifecycle:
postStart:
exec:
command: ["occ", "app:install notify_push"] And we have to active that plugin by running
|
I think it makes sense to give this option so the installation and setup is completely automatic, but I'd rather put it behind a second config flag: notify_push:
enabled: true
automatic_setup: true Maybe some people don't want to have this done automatically, so it's nice to give them the option. |
Therefore we has that hook of the container script (see #525), i write a ConfigMap for it. PS: the same way, then in #480 (@provokateurin you wanted to take a look there ...) PSS: Does somebody test this/my code? |
andre@server:~/k8s/nextcloud$ helm upgrade -n nextcloud akops-nextcloud -f values.yml ./helm/charts/nextcloud/
Error: UPGRADE FAILED: template: nextcloud/templates/notify_push/deployment.yaml:41:31: executing "nextcloud/templates/notify_push/deployment.yaml" at <$.Values.global.image.registry>: nil pointer evaluating interface {}.image |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I remove the global-references, then I got this error:
andre@server:~/k8s/nextcloud$ helm upgrade -n nextcloud akops-nextcloud -f values.yml ./helm/charts/nextcloud/
Error: UPGRADE FAILED: YAML parse error on nextcloud/templates/notify_push/deployment.yaml: error converting YAML to JSON: yaml: line 41: did not find expected key
Oh sorry, that was a copy-paste error. |
I was able to try an install. The result was this: Configuring Redis as session handler
=> Searching for scripts (*.sh) to run, located in the folder: /docker-entrypoint-hooks.d/before-starting
==> Running the script (cwd: /var/www/html): "/docker-entrypoint-hooks.d/before-starting/notify_push.sh"
notify_push already installed
✓ redis is configured
🗴 push server is not receiving redis messages (received 272721789, got 0)
==> Failed at executing "/docker-entrypoint-hooks.d/before-starting/notify_push.sh". Exit code: 1 EditI have a password for redis and it was not set. Can add this like this? - name: REDIS_URL
value: "redis://:<PASSWORD>@{{ template "nextcloud.redis.fullname" . }}-master:{{ .Values.redis.master.service.ports.redis }}" When I did this locally, then we have the chicken-egg-problem ... Maybe there is a better hook after nextcloud has started? |
okay, redis password should work:
PS: found that the redis password is not stored by default in a secret .... oh yes, that is a bootstrap problem. maybe we could remove it on one part. |
value: "http{{ if .Values.notifyPush.https }}s{{ end }}://{{ template "nextcloud.fullname" . }}.{{ .Release.Namespace }}.svc.cluster.local:{{ .Values.service.port }}" | ||
ports: | ||
- name: http | ||
containerPort: 7867 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
containerPort: 7867 | |
containerPort: 80 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that is strange - I do like to run pods with less priority and permission (port over 1000) and reNAT it with the service to an port like 80 ...
That is the reason why I set the ENV and this port to 7867.
So I like to debug, why the ENV does not work correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm also notify_push mean, this is the default port:
https://github.com/nextcloud/notify_push?tab=readme-ov-file#configuration
So, how should it work this way 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got a connection timeout when I started it and only saw the port 80 in the service. I was a bit in hurry.
Your right, this should be the correct port-setting.
With the fixed port, I still unable to run it. Logs from notify_push pod:
|
@wrenix I'm updated your push file to be |
|
||
# test the helm chart with notify push enabled | ||
- name: Notify Push Enabled | ||
helm_args: '--helm-extra-set-args "--set=notifyPush.enabled=true"' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to remove or tweak this @wrenix 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signed-off-by: WrenIX <[email protected]>
Signed-off-by: WrenIX <[email protected]>
Signed-off-by: WrenIX <[email protected]>
Signed-off-by: WrenIX <[email protected]>
…) - WIP Signed-off-by: WrenIX <[email protected]>
Signed-off-by: Jesse Hitch <[email protected]>
…can see test in ci Signed-off-by: Jesse Hitch <[email protected]>
d69d481
to
4e06db2
Compare
Pull Request
Description of the change
Not yet tested
Benefits
Possible drawbacks
Applicable issues
Additional information
Checklist
Chart.yaml
according to semver.TODO